extproc: register filter and parse base and override config#9073
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9073 +/- ##
==========================================
+ Coverage 80.52% 83.09% +2.56%
==========================================
Files 413 417 +4
Lines 33543 33648 +105
==========================================
+ Hits 27012 27959 +947
+ Misses 4316 4259 -57
+ Partials 2215 1430 -785
🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the Envoy external processing (ext_proc) HTTP filter for xDS, including configuration parsing for base and override settings. The reviewer noted that the implementation incorrectly uses a non-existent GRPC body processing mode instead of STREAMED and lacks support for TypedStruct types in the configuration parser. As a result, the validation logic and unit tests require updates to correctly handle supported processing modes and additional configuration formats.
|
LGTM, barring the udpa.type.v1.TypedStruct and xds.type.v3.TypedStruct support comment from gemini. Can go ahead if we get an answer on those. |
It has been discussed that we only have to support |
| } | ||
|
|
||
| // resolveHeaderMode resolves the processing mode for headers based on the | ||
| // protobuf enum value. If the mode is not set or set to Default, it returns the |
There was a problem hiding this comment.
Can we say ProcessingMode_DEFAULT in that case to be clear?
| return modeSend | ||
| case v3procfilterpb.ProcessingMode_SKIP: | ||
| return modeSkip | ||
| default: |
There was a problem hiding this comment.
Also, it might be better here and down in resolveBodyMode to have a case for ProcessingMode_DEFAULT and not have a default case. That way, if a new enum value is added, we don't silently treat it like the default.
There was a problem hiding this comment.
added a ProcessingMode_DEFAULT in the resolveHeaderMode but body mode does not have a ProcessingMode_DEFAULT and we need default statement because : As mentioned in the official GO documentation here , in point 6 , Switch is only terminating if it has a default case and all the cases including default has a return statement.
| // ChannelCredentials specifies the configuration for the transport | ||
| // credentials to use to connect to the external server, as a JSON string. | ||
| ChannelCredentials string | ||
| // CallCredentials specifies the configuration for the per-RPC credentials to | ||
| // use when making calls to the external server, as a JSON string. | ||
| CallCredentials string |
There was a problem hiding this comment.
If we are going to go with the approach where we are not going to bother to make this type be comparable, then, we should consider making the channel credentials be of type credentials.Bundle and the call credentials be of type credentials.PerRPCCredentials. (Note: see the xds/bootstrap package API for why we need a credentials.Bundle and not a credentials.TransportCredentials).
Then, as part of A102, when we parse the grpc_service proto into this struct, we can store a wrapped struct for the above fields, which embeds the credentials, but also stores the actual JSON configuration for the credentials.
And then, there needs to be an Equal method implemented here, that can actually compare the JSON configs of the credentials as part of the equality check.
What do you think?
There was a problem hiding this comment.
We can do that , but from what I could understand, we agreed for this to be part of A102 implementation , i.e. if we are already going to have a functionality that will take in the ServiceConfig and give us the grpc channel , behind the scene it will have ad to have some sort of mapping for the string and the actual transportCredentials. So I was thinking that we can keep the channelCreds and call creds as strings , and we can take the values from here to create a channelKey in the filter (or wherever needed) and directly compare them. But we can have an Equal method incase anyone wants to compare the whole ServiceConfig.
But what you suggested also makes sense if we first get the whole serviceConfig and then we make a copy of it where metadata and timeout are nil values and we use that as a key and then we can directly use the Equal method defined for the whole struct.
Let me know what you think.
There was a problem hiding this comment.
Ok, let's go with what we have for the moment, and fix it as we go along.
| // ChannelCredentials specifies the configuration for the transport | ||
| // credentials to use to connect to the external server, as a JSON string. | ||
| ChannelCredentials string | ||
| // CallCredentials specifies the configuration for the per-RPC credentials to | ||
| // use when making calls to the external server, as a JSON string. | ||
| CallCredentials string |
There was a problem hiding this comment.
Ok, let's go with what we have for the moment, and fix it as we go along.
3 participants
This PR is part of implementing A93: xds-ext-proc
This PR adds the ext_proc filter and the builder that implements the builder interface. That includes parsing and validating the base and the override config. The registration of the filter is under the
GRPC_EXPERIMENTAL_XDS_EXT_PROC_ON_CLIENTflag.#ext-proc-a93
RELEASE NOTES: None